fix(windows-rdp): escape special characters in RDP credentials - #1034
Open
matifali wants to merge 1 commit into
Open
fix(windows-rdp): escape special characters in RDP credentials#1034matifali wants to merge 1 commit into
matifali wants to merge 1 commit into
Conversation
Contributor
Module Scorecard Check
|
| Theme | Before | After |
|---|---|---|
| Presentation & Onboarding | 21 / 25 | 17 / 25 |
| Integration | — | — |
| Credential Hygiene | 10 / 20 | 18 / 20 |
| Restricted-Environment | 0 / 20 | 0 / 20 |
| Engineering Quality | 7 / 10 | 10 / 10 |
| Overall | 51 / 100 | 60 / 100 |
Full scorecard for this PR
| Presentation & Onboarding | Credential Hygiene | Restricted-Environment Readiness | Engineering Quality | Overall |
|---|---|---|---|---|
| 17 / 25 | 18 / 20 | 0 / 20 | 10 / 10 | 60 / 100 |
Drilldown
Presentation & Onboarding — 17 / 25
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Configuration-mode examples | 12 | 12 | README provides examples for AWS and Google Cloud providers, plus a custom version example. Each shows sensible defaults for the major configuration mode (specifying Devolutions Gateway version). |
| Coder-context framing | 8 | 0 | README does not explain what the module adds on top of Coder or how Coder fits in the RDP flow. It names the target tool (Devolutions Gateway) but lacks context about Coder's role in enabling web-based RDP access. |
| Visual preview | 5 | 5 | README includes a video thumbnail image with a link to a video demonstration. |
Credential Hygiene — 18 / 20
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Secrets marked sensitive | 16 | 16 | The admin_password variable is marked sensitive = true in main.tf. README examples do not inline secrets; they rely on the module's default value. |
| Non-hardcoded auth path | 4 | 2 | The module uses a default password approach rather than demonstrating integration with external auth systems, IAM, or secret managers. While the password is configurable, the README doesn't show a path that avoids pasting credentials into templates. Half credit for the configurable approach being better than fully hardcoded. |
Restricted-Environment Readiness — 0 / 20
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Mirrorable artifact source | 10 | 0 | No module input variable overrides the PowerShell Gallery URL or Devolutions Gateway download source. The devolutions_gateway_version variable only controls version selection, not the artifact source location. The installation script hardcodes PSGallery and the module's own download mechanisms. |
| Bring-your-own binary | 5 | 0 | No documented way to skip the Devolutions Gateway installation when it's pre-installed in the image. The module always runs Install-Module and Install-DGatewayPackage. |
| Egress transparency | 3 | 0 | No dedicated README section enumerating external endpoints. The PowerShell script contacts PSGallery and Devolutions package sources, but these are only inferable from code, not documented. |
| Runs without sudo | 2 | 0 | The PowerShell script performs system-wide installations (Install-Module for all users, service configuration with Set-Service, registry modifications with New-ItemProperty on HKLM, and firewall rules with Enable-NetFirewallRule). These operations require administrator privileges on Windows. No documented non-admin path exists. |
Engineering Quality — 10 / 10
| Criterion | Max | Score | Notes |
|---|---|---|---|
| Input quality | 6 | 6 | Variables have clear descriptions, sensible defaults (display_name, slug, icon, admin credentials, version). The share variable includes validation. The devolutions_gateway_version variable has a helpful description explaining 'latest' vs specific versions. |
| Test coverage | 4 | 4 | Comprehensive TypeScript test suite in main.test.ts covers business logic: verifies PowerShell script installation commands, validates username/password injection into JS patch file, tests special character preservation in passwords. Tests cover both default and custom credential scenarios. |
Overall — 60 / 100
Raw 45 / 75 → round(45 / 75 × 100) = 60
Scored against SCORECARD.md with claude-sonnet-4-5. Language-model scores are advisory.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The password shown in the Coder UI and the password that reaches the Devolutions login form can differ, because the module interpolates credentials into two different languages without escaping either of them:
devolutions-patch.jsembeds the password inside a double-quoted JS string literal (value: "${CODER_PASSWORD}"). JS parses the injected\mas an escape sequence and drops the backslash, soN;JVO*U\mL^a*PbecomesN;JVO*UmL^a*P. A"in the password breaks the patch script entirely.powershell-installation-script.tftplpasses the password as a PowerShell double-quoted string (Set-AdminPassword -adminPassword "${admin_password}"), where$, backtick, and"are all interpreted. The Windows account then gets a different password than the one Coder displays.Both paths end in
STATUS_LOGON_FAILURE [0xc000006d], and whether a user hits it depends on which characters their generated password happens to contain.Fix
Encode each value for its destination:
jsonencode, outer quotes trimmed\,", control chars, and<>&as\u003cstyle escapes'doubled$, backtick,",\, and everything else literallyThe JS file keeps a real string literal, so it stays valid JavaScript for Prettier and
@ts-check.Changes
main.tf: addlocalsthat JSON-escape credentials for the JS patch and PowerShell-escape them for the installation scriptpowershell-installation-script.tftpl: use single-quoted strings for the username and passwordmain.test.ts: parse the emitted JS literal withJSON.parseinstead of a naive[^"]+regex, and add a case covering\ " '$ & < > | { } [ ] % @ : ~`1.3.1Validation
Rendered script output with password
N;JVO*U\mL^a*P"'+`+$&<>|:bun test main.test.tspasses (5/5);prettierandterraform fmtare clean.Note
amazon-dcv-windowshas the same class of bug ($adminPassword = "${admin_password}"ininstall-dcv.ps1, and an unencodedpassword=query parameter inmain.tf). Filing that separately rather than widening this PR.Closes #20